Skip to content

incremental builds: add derivation override functions#167670

Merged
dasJ merged 16 commits intoNixOS:masterfrom
messemar:incremental-builds
Dec 19, 2023
Merged

incremental builds: add derivation override functions#167670
dasJ merged 16 commits intoNixOS:masterfrom
messemar:incremental-builds

Conversation

@messemar
Copy link
Contributor

@messemar messemar commented Apr 7, 2022

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Apr 7, 2022
@Artturin Artturin requested a review from roberth April 7, 2022 12:41
@roberth
Copy link
Member

roberth commented Apr 7, 2022

Intriguing!

Some thoughts:

  • buildOut: "out" is a little redundant
  • the intermediate files are more likely not to be reproducible than normal outputs
  • most of all: Please write tests

@messemar messemar marked this pull request as draft April 7, 2022 14:30
@messemar messemar marked this pull request as ready for review April 8, 2022 09:15
@messemar
Copy link
Contributor Author

messemar commented Apr 8, 2022

Intriguing!

Some thoughts:

* buildOut: "out" is a little redundant

* the intermediate files are more likely not to be reproducible than normal outputs

* most of all: Please write tests

Thanks for your input.

I have changed the names and added a test. Let me know If there is more to do for testing.

To your second concern:
I guess the intermediate files should be as reproducible as normal outputs, as they depends on the input and the compiler, also.
But I can not guarantee that they will be reproducible at any time.
I run the same derivation several times, and the derivation would not be rebuilt if nothing in the source tree changes.

@messemar messemar force-pushed the incremental-builds branch 2 times, most recently from 0080780 to bfe0a0c Compare April 8, 2022 09:27
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Apr 8, 2022
@tfc
Copy link
Contributor

tfc commented Apr 8, 2022

This is pretty cool.

Interface wise i think it might be nicer to use and it seems like nothing speaks against doing it like this maybe?

incrementalDrv = pkgs.buildIncremental drv;

this could be done by implementing build-incremental.nix like this:

{ pkgs }:
let
  prepareIncrementalBuild = drv: ...;
  mkIncrementalBuild = drv: ...;

in

drv: previousArtifacts:

mkIncrementalBuild (prepareIncrementalBuild drv) previousArtifacts

The tests would look slightly different but would show the same thing.

@messemar
Copy link
Contributor Author

messemar commented Apr 8, 2022

This is pretty cool.

Interface wise i think it might be nicer to use and it seems like nothing speaks against doing it like this maybe?

incrementalDrv = pkgs.buildIncremental drv;

this could be done by implementing build-incremental.nix like this:

{ pkgs }:
let
  prepareIncrementalBuild = drv: ...;
  mkIncrementalBuild = drv: ...;

in

drv: previousArtifacts:

mkIncrementalBuild (prepareIncrementalBuild drv) previousArtifacts

The tests would look slightly different but would show the same thing.

You can not handle it that way, because there is no possibility to detect an earlier build automaticly, as nix is lacking the corresponding Support.

@tfc
Copy link
Contributor

tfc commented Apr 8, 2022

You can not handle it that way, because there is no possibility to detect an earlier build automaticly, as nix is lacking the corresponding Support.

Ok sorry for the noise. Now i get how it works.

@roberth
Copy link
Member

roberth commented Apr 11, 2022

  • What happens when a source file is removed in the new build? It seems unavoidable that the accompanying intermediate file will be present, which is a risk, but a decent build script will only glob for source files.
    This needs a test case.

  • In the existing test case, could you check that unmodified files aren't recompiled?

@messemar
Copy link
Contributor Author

  • What happens when a source file is removed in the new build? It seems unavoidable that the accompanying intermediate file will be present, which is a risk, but a decent build script will only glob for source files.
    This needs a test case.
    Yes, good Idea, I will add it
    • In the existing test case, could you check that unmodified files aren't recompiled?

Yes.

@messemar messemar force-pushed the incremental-builds branch from bfe0a0c to d9335dc Compare April 13, 2022 07:51
@messemar
Copy link
Contributor Author

I've added the requested tests.

@messemar messemar marked this pull request as draft April 13, 2022 13:20
@YellowOnion
Copy link
Contributor

Does this require two copies of src? this doesn't seem to create genuine incremental builds, but require one "unaltered" source, that acts as a seed for the 2nd derivation. This can be problematic when you want to work on big projects like linux source code.

@roberth
Copy link
Member

roberth commented May 30, 2022

Does this require two copies of src?

Yes.

this doesn't seem to create genuine incremental builds

That is correct. "True" incremental builds are a cross-cutting concern that goes all the way down to the build tool used inside the sandbox. As such, Nix can not be solely responsible for managing the state, making such a solution fragile, unless the build tool can delegate the state management to recursive nix, which is, still, rather restrictive and an invasive requirement.
Incrementality can't be added in a transparent manner, so instead, this is an embodiment of an incremental build within the derivation paradigm, that makes the prior state explicit and does not provide any guarantees beyond the fact that the result is an incremental build from a specific version to another specific version.

@messemar Could you explain this in the Nixpkgs manual?

This can be problematic when you want to work on big projects like linux source code.

If you buy into Nix, you have to be ok with "wasting" some space for the benefits of reproducibility, atomicity, etc.
There's two kinds of space waste: the temporary and the permanent. The first just causes garbage collection to run more frequently, the latter should not be affected by this solution.
When you're rebuilding, say, linux you are already copying the src to the store for each new iteration (unless you only change the expression), so presumably the garbage produced isn't much of a problem.
I guess I'm skipping over some subtleties, but I think this explanation is already getting quite long.

@messemar
Copy link
Contributor Author

@messemar Could you explain this in the Nixpkgs manual?

Yes I can, when I have some spare time again I will. As well as continuing the implementation, of course :D

@messemar messemar force-pushed the incremental-builds branch from d9335dc to 98c1c46 Compare June 20, 2022 10:20
@messemar
Copy link
Contributor Author

Steps to un-draft: Add a section in the nixpkgs manual.

Copy link
Member

@dasJ dasJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good and functionality is lovely to have (would have helped me bisecting the kernel a couple of days ago :)

Co-authored-by: Philipp Schuster <phip1611@gmail.com>
@messemar
Copy link
Contributor Author

@roberth @dasJ Is there anything left, that I can do to get this over the finishing line?

@dasJ
Copy link
Member

dasJ commented Dec 15, 2023

There isn't

@dasJ
Copy link
Member

dasJ commented Dec 15, 2023

Ah there is: @infinisil is it okay to dismiss your review?

outputs = [ "out" ];
name = drv.name + "-checkpointArtifacts";
# To determine differences between the state of the build directory
# from an earlier build and a later one we store the state of the build
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
# from an earlier build and a later one we store the state of the build
# from an earlier build and a later one we store the state of the build

cp -r ./* $out/sources/
'';

# After the build the build directory is copied again
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# After the build the build directory is copied again
# After the build, the build directory is copied again

# After the build the build directory is copied again
# to get the output files.
# We copy the complete build folder, to take care for
# Build tools, building in the source directory, instead of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Build tools, building in the source directory, instead of
# build tools, building in the source directory, instead of

# to get the output files.
# We copy the complete build folder, to take care for
# Build tools, building in the source directory, instead of
# having a build root directory, e.G the Linux kernel.
Copy link
Member

@phip1611 phip1611 Dec 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# having a build root directory, e.G the Linux kernel.
# having a build root directory, e.g., the Linux kernel.

or

Suggested change
# having a build root directory, e.G the Linux kernel.
# having a build root directory, such as the Linux kernel.

Copy link
Member

@phip1611 phip1611 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Left some nits.

@dasJ dasJ dismissed infinisil’s stale review December 19, 2023 10:37

Stale, everything appears to be fixed

@dasJ dasJ merged commit 5eed541 into NixOS:master Dec 19, 2023
@infinisil
Copy link
Member

infinisil commented Dec 19, 2023

Ah there is: @infinisil is it okay to dismiss your review?

Oh yeah sorry missed this. It's okay. Generally this does seem a rather hacky, but it looks like it's good enough for a lot of use cases, so it's fine.

@messemar: Though it looks like #167670 (comment) wasn't fully addressed, and I can't find a comment explaining why. Not very important though.

@roberth
Copy link
Member

roberth commented Dec 19, 2023

rather hacky

I'm sure you think the same about it, but to be clear, the goal itself is hacky, as something non-hacky would have to integrate with something other than phases perhaps, in order for it to be able to give guarantees that can't be messed up by the user of this feature, but it's unclear to what that would be.
The implementation is not at all hacky in and of itself.

@messemar
Copy link
Contributor Author

@infinisil and @roberth
I totally agree on the hackyness. But as checkpoints and incremental builds are not a first class concept for nix, it can not be enabled in a non hacky way, I think.

Regarding #167670 (comment) If we use dotglob wouldn't that mean that we also remove ´.´ and ´..´ as well ? Does not sound like a good Idea IMHO. But yes, we might miss dotfiles then.

@infinisil
Copy link
Member

infinisil commented Dec 19, 2023

@messemar You do have dotglob enabled in the PR. I was thinking of the other point in that comment that allows avoiding the extglob for rm -r !("sourceDifference.patch").

Which would then also work if the source has a sourceDifference.patch file itself.

@messemar
Copy link
Contributor Author

@infinisil Ah, now I get it. Is it a nit, or should I fix it in a follow up PR ?

@infinisil
Copy link
Member

@messemar I'd classify it as a nice-to-have for correctness. Feel free to make a follow-up PR, but not necessarily :)

@asymmetric
Copy link
Contributor

asymmetric commented Dec 30, 2023

Would it make sense to mention this in the 2023 recap? Or is it not supposed to be seen by too many people? 🐱

@roberth
Copy link
Member

roberth commented Dec 30, 2023

@asymmetric It needs an explicit starting state, and it only works for typical mkDerivation based builds. That should be made very clear. I would also like to see it used in practice somewhere.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/idea-mkderivation-auto-generate-unwrapped-package-right-after-build/37901/3

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/idea-mkderivation-auto-generate-unwrapped-package-right-after-build/37901/1

@bryango
Copy link
Member

bryango commented Jan 7, 2024

I really, really like this idea and would like to use it in practice! However, there are some minor inconsistencies across the docs; e.g. mkCheckpointBuild is mentioned in the doc:

});
in checkpointBuildTools.mkCheckpointBuild changedHello helloCheckpoint
```

But the actual command is mkCheckpointedBuild with an ed.

In fact, all other functions in this PR are in the form of *CheckpointBuild* without the ed, so I think we might as well trade the mkCheckpointedBuild name in favor of mkCheckpointBuild for a uniformed look (and link mkCheckpointedBuild to mkCheckpointBuild with a lib.warn "deprecated", although currently there is no usage of this in nixpkgs). Is that okay? If so, I can make a PR!

Update: I made a draft here, along with other cosmetic changes.

@messemar
Copy link
Contributor Author

messemar commented Jan 7, 2024

In fact, all other functions in this PR are in the form of CheckpointBuild without the ed, so I think we might as well trade the mkCheckpointedBuild name in favor of mkCheckpointBuild for a uniformed look (and link mkCheckpointedBuild to mkCheckpointBuild with a lib.warn "deprecated", although currently there is no usage of this in nixpkgs). Is that okay? If so, I can make a PR!

@bryango From my perspective, the change is reasonable and I like it.

I would love to see a PR for this.

bryango pushed a commit to bryango/nixpkgs that referenced this pull request Jan 10, 2024
incremental builds: add derivation override functions

(cherry picked from commit 5eed541)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1.severity: significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. 8.has: documentation This PR adds or changes documentation 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.